PHPC-752: Add maxStalenessMS to ReadPreference class #400

jmikola · 2016-09-12T21:23:14Z

https://jira.mongodb.org/browse/PHPC-752

I was premature in signing off on #380, as it was lacking support for maxStalenessMS on the ReadPreference object. This adds a new constructor argument, getter method, and adds "maxStalenessMS" to the ReadPreference debug output.

jmikola · 2016-09-12T21:28:16Z

tests/readPreference/readpreference-ctor-001.phpt

-var_dump(new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_SECONDARY, array(array("tag" => "one"))));
-var_dump(new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_PRIMARY, array()));
+var_dump(new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_SECONDARY, [['tag' => 'one']]));
+var_dump(new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_PRIMARY, []));


Just array() to [] refactoring here. Since this is the only ReadPreference constructor test, I added a new case for "maxStalenessMS" below; however, it's probably redundant in light of the ReadPreference::getMaxStalenessMS() test.

derickr · 2016-09-15T15:21:14Z

src/MongoDB/ReadPreference.c

@@ -46,21 +46,22 @@ PHONGO_API zend_class_entry *php_phongo_readpreference_ce;

 zend_object_handlers php_phongo_handler_readpreference;

-/* {{{ proto void ReadPreference::__construct(integer $mode[, array $tagSets = array()])
+/* {{{ proto void ReadPreference::__construct(integer $mode[, array $tagSets = array()[, integer $maxStalenessMS]])


I would much rather see this being an options array, as it is possible that other options might be added in the future.

I'll refactor this to expect an options array and check a "maxStalenessMS" value.

derickr · 2016-09-15T15:22:13Z

tests/readPreference/readpreference-getTagSets-001.phpt

@@ -1,5 +1,5 @@
 --TEST--
-MongoDB\Driver\ReadPreference::getMode()
+MongoDB\Driver\ReadPreference::getTagSets()


This commit doesn't really belong here, perhaps in #399.

It's a stand-alone commit akin to any typo fix. The PR should make no difference.

Moved to #408.

jmikola · 2016-09-15T20:19:41Z

src/MongoDB/ReadPreference.c

-		case 2:
-			if (tagSets) {
-				bson_t *tags = bson_new();
+	if (tagSets) {


Since I removed the switch statement and am processing tagSets first, I added a test that tag set exceptions are thrown before maxStalenessMS.

jmikola · 2016-09-15T20:21:23Z

tests/manager/manager-ctor-read_preference-003.phpt

@@ -0,0 +1,54 @@
+--TEST--
+MongoDB\Driver\Manager::__construct(): read preference options of the wrong type are ignored


We may want to change this behavior in the future, but this is how processing of Manager options operates today. We can revise this test if we decide to change this in the future.

I'll note that mongoc-uri.c is also quite lax about type errors. For instance, options that are expected to be integers are run through strtol() with no error handling.

jmikola · 2016-09-15T20:23:00Z

tests/manager/manager-maxstalenessms-001.phpt

@@ -1,13 +0,0 @@
--TEST--
-MongoDB\Driver\Manager: maxStalenessMS


This was essentially renamed to manager-ctor-read_preference-002.phpt, where I added an additional test case and assert the state of Manager::getReadPreference().

jmikola · 2016-09-15T20:23:42Z

tests/manager/manager-maxstalenessms_error-001.phpt

@@ -1,33 +0,0 @@
--TEST--
-MongoDB\Driver\Manager: maxStalenessMS


This was renamed to manager-ctor-read_preference-error-002.phpt, where I added an additional error case for an out-of-range value.

derickr

Looks good, except for the inclusion of maxStalenessMS in debug output when it's not set, or using the default.

derickr · 2016-09-16T10:19:49Z

tests/manager/manager-ctor-read_preference-001.phpt

+--EXPECTF--
+object(MongoDB\Driver\ReadPreference)#%d (%d) {
+  ["mode"]=>
+  int(1)


This is going to fail/create conflicts after the PHPC-498 changes (ReadPreference, ReadConcern, and WriteConcern value).

Is that because you're changing it to display the string name? If so, you can easily update this when PHPC-489 is implemented.

I have absolutely no reservations whatever about going back and modifying tests.

Yes, it turns into a string. I'll do the PHPC-489 tomorrow (Monday), should be ready for you to review when you wake up.

derickr · 2016-09-16T10:20:52Z

php_phongo.c

@@ -982,6 +982,8 @@ void php_phongo_read_preference_to_zval(zval *retval, const mongoc_read_prefs_t
 	} else {
 		ADD_ASSOC_NULL_EX(retval, "tags");
 	}
+
+	ADD_ASSOC_LONG_EX(retval, "maxStalenessMS", mongoc_read_prefs_get_max_staleness_ms(read_prefs));


I don't think this should be added if maxstaleness is the default (0), just like we're no longer to do that for "journal", "w", etc.

derickr · 2016-09-16T10:21:20Z

tests/manager/manager-ctor-read_preference-001.phpt

+  array(0) {
+  }
+  ["maxStalenessMS"]=>
+  int(0)


I think this should be excluded here (As mentioned in an earlier comments).

derickr · 2016-09-19T11:06:48Z

php_phongo.c

@@ -963,7 +963,7 @@ void php_phongo_read_preference_to_zval(zval *retval, const mongoc_read_prefs_t
 {
 	const bson_t *tags = mongoc_read_prefs_get_tags(read_prefs);

-	array_init_size(retval, 2);
+	array_init_size(retval, 3);


I guess using 3 when there are only 2 items is fine, right?

This just controls the initial HashTable size, so it should be fine. The buckets are still empty until we fill them.

This adds an options array to the ReadPreference constructor, which accepts a maxStalenessMS option. The option is also exposed via a getter method and in debug output.

This also adds various tests for parsing of read preference Manager options.

jmikola mentioned this pull request Sep 12, 2016

PHPC-424: Validate that RP tag set is an array of documents #399

Merged

jmikola reviewed Sep 12, 2016
View reviewed changes

jmikola force-pushed the phpc-752 branch from a5d2fda to e95667b Compare September 12, 2016 21:32

derickr reviewed Sep 15, 2016

View reviewed changes

jmikola force-pushed the phpc-752 branch 5 times, most recently from 8e37c11 to 8bb7d70 Compare September 15, 2016 20:18

jmikola commented Sep 15, 2016

View reviewed changes

derickr suggested changes Sep 16, 2016

View reviewed changes

jmikola force-pushed the phpc-752 branch 2 times, most recently from 2b62d20 to a9e9776 Compare September 16, 2016 14:06

derickr approved these changes Sep 19, 2016

View reviewed changes

jmikola mentioned this pull request Sep 19, 2016

PHPC-786: Tests for appname URI option #412

Closed

jmikola force-pushed the phpc-752 branch from a9e9776 to 9d150ce Compare September 19, 2016 20:52

jmikola added 7 commits September 19, 2016 16:54

Remove unnecessary SKIPIFs from ReadPreference tests

d9b6613

Use short array syntax in ReadPreference tests

40a0f52

PHPC-752: Add maxStalenessMS to ReadPreference class

77209dd

This adds an options array to the ReadPreference constructor, which accepts a maxStalenessMS option. The option is also exposed via a getter method and in debug output.

Rename Manager ctor read preference and write concern tests

701e07f

Remove unnecessary SKIPIFs from Manager ctor tests

eeb7c04

PHPC-752: Validate range of maxStalenessMS Manager option

0160871

This also adds various tests for parsing of read preference Manager options.

Test that read preference options of the wrong type are ignored

6ec5348

jmikola force-pushed the phpc-752 branch from 9d150ce to 6ec5348 Compare September 19, 2016 20:54

jmikola merged commit 6ec5348 into mongodb:master Sep 19, 2016

jmikola added a commit that referenced this pull request Sep 19, 2016

Merge pull request #400

4a51f72

jmikola deleted the phpc-752 branch September 19, 2016 20:55

		@@ -0,0 +1,54 @@
		--TEST--
		MongoDB\Driver\Manager::__construct(): read preference options of the wrong type are ignored

		@@ -1,13 +0,0 @@
		--TEST--
		MongoDB\Driver\Manager: maxStalenessMS

		@@ -1,33 +0,0 @@
		--TEST--
		MongoDB\Driver\Manager: maxStalenessMS

PHPC-752: Add maxStalenessMS to ReadPreference class #400

PHPC-752: Add maxStalenessMS to ReadPreference class #400

Uh oh!

Conversation

jmikola commented Sep 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmikola commented Sep 12, 2016 •

edited

Loading